-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Now that WP_Query is cached, just load all templates. #47253
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey Can you explain what the benefits of this would be?
Without additional context, just from looking at this I have some concerns:
- I don't see the benefits as it is still one query, like before.
- The query however now queries all template parts instead of just one.
- The unbound query with
posts_per_page = -1
has me concerned about potential problems. - The extra logic to go through all found posts also seems unnecessary, the previous logic of querying just for the one post is more efficient.
So I would like to understand better why we should be doing this.
I'm guessing the intended benefit here is for pages with multiple template parts, for example a header and footer. On the first call both template-parts would be queried from the database (presuming no persistent object cache) but on the second call there will not be a database query as the result is cached. I guess the concern is for sites that do not have a persistent cache and use different template-parts in a variety of templates (for example a different header on the home and internal pages). With this change, there would be more data returned on each page load. Pros
Cons
|
Thanks @peterwilsoncc, makes sense. I like the idea behind it, but I'm also concerned about a few things: As you're saying, depending on how sites use block template parts, this PR may make performance better or worse. About sites with a persistent cache, it would seem to me that the benefits are higher than for sites without one, since the query would allow avoiding queries on the other block templates. On the other hand, one could argue that this benefit is tiny or even non existent when looking at the big picture. Consider the following:
We may need data for those different scenarios to better assess overall impact. Right now I'm questioning whether this change is worthwhile. |
So this is the point, how well does this scale. Sites with lots of template parts are more likely to need one more than template part. So header, footer, sidebar, comments etc. So more likely to need more template parts.
Looping through data in PHP is extremely quick, to the point of not mention it's effect on performance. The much bigger hit to performance is the database query and the overhead of that. Not sure this should end up in core, we may need to be a single query to get all template parts we know are in core. Doing lots of single queries means the following.
This PR needs testing but I think still worth exploring. |
I thought I'd do some testing with a mini plugin to compare the proposed with the current code. https://gist.github.com/peterwilsoncc/50222356f2549797db8e224af46b1128 When using a subset of template parts, two of eight, I see a consistently improved performance on the test with a cold cache but slower performance on the test with a warm cache. I'm testing with Chassis (vagrant on VirtualBox 6) which may be affecting it. Vagrant has taken the faster boot speed, slower to run compromise which is the reverse of Docker. Playing around with the constants should give you an idea of various cases. When changing the number of templates or number of templates used, I recommend ignoring the first result as it will have done a bunch of work up-front that may affect the rest of the run. Also, please let me know if the mini plugin is flawed, it's always possible I've fluffed something up. |
@spacedmonkey - just looping back to this stale PR to see if it is something you're still pursuing or if it can be closed? |
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
What?
See #45309
Now that
WP_Query
is cached WordPress/wordpress-develop@7f7d616, we should just load all template parts and use php to filter.Before
After
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast